Conversation
tanguyenvn
changed the title
…from simplewebauthn
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…ing vault key protection
tanguyenvn
force-pushed
the
feat/TO-540-passkey-controller
branch
from
a7e07f8 to
5cc58c9
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5cc58c9. Configure here.
mcmire
left a comment
mcmire
left a comment
There was a problem hiding this comment.
I haven't looked over the implementation of this package (if I have time, I will do that later). I just left comments based on standards for controllers and new packages in general.
| }); | ||
| if (!verified || !registrationInfo) { | ||
| this.#ceremonyManager.deleteRegistrationCeremony(challenge); | ||
| throw new Error('Passkey registration verification failed'); |
There was a problem hiding this comment.
make error msgs constants or use error class
tanguyenvn
force-pushed
the
feat/TO-540-passkey-controller
branch
from
af68e38 to
8a3bf04
Compare
mcmire
left a comment
mcmire
left a comment
There was a problem hiding this comment.
I'm curious whether it makes sense to extract the WebAuthn code to a separate package in the future. It seems like it could be valuable for other use cases. Something to think about.
In the meantime this looks good to me.
|
@mcmire the webauthn code is ported from @simplewebauthn/server package. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a3bf04720
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| encKey: Uint8Array; | ||
| keyDerivation: PasskeyKeyDerivation; | ||
| } { | ||
| const credentialId = registrationResponse.id; |
There was a problem hiding this comment.
deriveKeyFromRegistrationResponse derives the vault wrapping key from registrationResponse.id, which is not covered by the attestation signature, while the persisted credential ID later comes from parsed authenticator data. A malformed/tampered response where id/rawId differs from the attested credential ID will still pass verification and produce an encryption key tied to the wrong salt, permanently breaking subsequent unlock/decrypt for that enrollment. Derive from the verified attested credential ID (or explicitly reject mismatches) before encrypting and storing state.
Useful? React with 👍 / 👎.
| residentKey: 'preferred', | ||
| }, | ||
| hints: ['client-device', 'hybrid'], | ||
| attestation: 'direct', |
There was a problem hiding this comment.
Registration options always request attestation: 'direct', but verification only accepts fmt === 'none' or packed self-attestation and explicitly rejects packed x5c chains/other direct formats. A compliant authenticator can legitimately return those direct attestations, so enrollment will fail on those devices even though the browser followed requested options. Either request 'none' here or add verifier support for the direct attestation formats you are asking clients to produce.
Useful? React with 👍 / 👎.
This reverts commit 819f0c4.
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
@metamaskbot publish-previews |
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
5 participants
Description
Introduces
@metamask/passkey-controller, aBaseController-backed package that orchestrates WebAuthn passkey enrollment and authentication for vault key protection: generating ceremony options, verifying authenticator responses, HKDF-based key derivation (PRF vsuserHandle), AES-256-GCM wrapping of the vault encryption key, renewal flows for password change, and state/ceremony management (including concurrent ceremonies and lifecycle clears).Also wires the package into the monorepo (workspace/tsconfig,
CODEOWNERS,teams.json, root README, lockfile).Notes for reviewers (SimpleWebAuthn)
Extension vs Core split
@simplewebauthn/browserfor client-side helpers that run wherenavigator.credentialsis available.@simplewebauthn/server: that package targets Node-oriented verification APIs and assumptions that do not fit the constrained extension background environment the same way.To keep verification logic aligned with SimpleWebAuthn while staying dependency-appropriate in Core, this package inlines / ports the relevant server-side verification behavior (registration and authentication response verification, signature verification, CBOR/WebAuthn parsing helpers) into
packages/passkey-controller/src/webauthn/rather than adding@simplewebauthn/serveras a runtime dependency. When reviewing, please treat those modules as parity-sensitive: changes should stay consistent with upstream SimpleWebAuthn semantics where we intentionally mirror them (see recent commits around verification parity).Changelog
packages/passkey-controller/CHANGELOG.md— follow monorepo changelog rules; ensureyarn validate:changelogpasses before merge.Related issues
Fixes:
Testing
yarn workspace @metamask/passkey-controller run testyarn workspace @metamask/passkey-controller run buildManual testing (consumers)
N/A for Core in isolation; extension integration should exercise registration, unlock, change-password / vault key renewal, wallet reset, and edge cases called out in the extension PR.
Note
High Risk
Introduces new WebAuthn verification and cryptography (HKDF/AES-GCM) for protecting and renewing the vault encryption key, which is security-critical and easy to get subtly wrong. Also adds in-memory ceremony state handling and error-code semantics that consumers must integrate correctly.
Overview
Adds a new
@metamask/passkey-controllerpackage that orchestrates WebAuthn passkey enrollment/authentication to wrap/unwrap the vault encryption key, including PRF vsuserHandlekey derivation (HKDF-SHA256) and AES-256-GCM vault-key protection plus a renewal flow for password changes.Implements and tests supporting infrastructure: challenge-keyed
CeremonyManagerwith TTL/capacity controls, internal WebAuthn parsing/verification helpers (e.g., authenticator data parsing, RP ID hash matching, signature verification), and a structuredPasskeyControllerErrorwith stable errorcode/message.Wires the new package into the monorepo via
README.mdpackage listing/dependency graph updates andCODEOWNERSentries (including release-file ownership for the new package).Reviewed by Cursor Bugbot for commit 4c08463. Bugbot is set up for automated code reviews on this repo. Configure here.